Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

Started handling messages from background isolates for iOS #35174

Merged
merged 19 commits into from
Sep 8, 2022

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Aug 4, 2022

This provides all the work for handling platform channels in the Dart->Host direction for iOS. For more details and integration tests see the framework PR.

companion framework pr: flutter/flutter#109005
issue: flutter/flutter#13937

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gaaclarke gaaclarke force-pushed the isolate-platform-channels branch 17 times, most recently from 7b70a06 to 510bc0a Compare August 12, 2022 00:15
@gaaclarke gaaclarke force-pushed the isolate-platform-channels branch 5 times, most recently from 1bee788 to f303fba Compare August 12, 2022 20:04
@gaaclarke
Copy link
Member Author

gaaclarke commented Aug 12, 2022

@jason-simmons Can you please have a look at this draft PR? No need to approve it. I just want to make sure we are on a happy path. This PR follows the work outlined in the Isolate Platform Channel design doc and works for iOS Dart->Host messages (notice there is a framework PR if you want to peek at that too). It probably needs a bit more documentation in the source code. I'm still working on a testing plan, your feedback will help inform that. I'm going to have an integration test in the flutter repo for sure.

Something you might find interesting is the threadsafe_unique_ptr. I came up with that with you in mind since I wanted weak_ptr but you've had some valid concerns about shared_ptr being hard to verify so I think this addresses those in this case where we want a clear single owner.

My plan for this is to get early feedback from you and if everything goes well I'll use this as the template for implementing Dart->Host isolate platform channels across the platforms. I don't have plans on implementing Host->Dart isolate platform channels anytime soon (since it didn't have a clear resolution in the design doc and isn't what most people are asking for).

@gaaclarke gaaclarke force-pushed the isolate-platform-channels branch from f303fba to f7d00a5 Compare August 12, 2022 20:16
@jason-simmons
Copy link
Member

I'm concerned that allowing access to the root isolate's PlatformConfiguration from multiple threads and isolates will be too confusing and error-prone. Likewise for allowing access to UI thread related Shell services from threads besides the UI thread.

I'm not sure what would be the safest way to implement this. The first potential option that comes to mind would be populating the child isolate's UIDartState::Context with the platform task runner plus a service that can be invoked on the platform thread to dispatch a platform message.
The service would need to hold a weak reference to the PlatformView and wrap PlatformView::HandlePlatformMessage. The service could be passed from the parent isolate to the child isolate via the DartIsolateGroupData.

Would be interested in getting consensus among the engine team about how best to do this (@chinmaygarde @dnfield)

@gaaclarke gaaclarke force-pushed the isolate-platform-channels branch from 6f0dafc to 9cc8567 Compare September 7, 2022 21:55
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if LGT @jason-simmons

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2022
@auto-submit auto-submit bot merged commit 2be73ba into flutter:main Sep 8, 2022
@gaaclarke
Copy link
Member Author

Thanks everyone, I'll get to work cleaning up and putting and the dependent PRs into review today.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants